Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Tables #8

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

jimbo8098
Copy link
Contributor

The second part of the saga, merged with master as commited the other day.

The PR brings OOified tables, fixes the bug in #7 , better null variable handling and uses a BadgeDomain to handle queries. This means it relies on GibbonEdu/core#889 being approved because that includes the domain it uses.

Jim Speir and others added 12 commits May 16, 2019 17:18
Also adding the $gibbon variable to the data sent to getBadges. This is mainly for use of session variables but may be expanded on later on.
… variable, falling back to $_SESSION (old style)

- badges_manage now OOified
- badges_grant now OOified
- Added expandable column for comment on both badges_grant and badges_manage
- Set the width of the badge image to 155px and text to be italic and centred in the column.
- Removed TODOs.
@SKuipers SKuipers self-requested a review May 22, 2019 01:00
@SKuipers
Copy link
Member

Awesome, looks like some great progress! It looks like you may have worked on this branch in parallel to the forms 🤔 it appears to be dialing the version increment back rather than forward. Your namespaces also now likely need updated based on the suggested changes from GibbonEdu/core#889

@jimbo8098
Copy link
Contributor Author

Namespaces updated, I'll look at the form side of things tomorrow.

@jimbo8098
Copy link
Contributor Author

Ok I've merged in the changed from before and removed unnecessary changes to the CSS file for good measure. Version has been incremented and changelog updated.

@SKuipers SKuipers self-assigned this May 28, 2019
Copy link
Member

@SKuipers SKuipers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good. It appears that in the meantime Ross has leap-frogged you and jumped up to v2.6.00 with some added functionality 😂 It appears that this should work fine with the new changes, but I'm happy to leave that merge to you. Give a shout once you've handled the changes and bumped things up to v2.6.01, then I'll take another look to merge this. Thanks!!

Base automatically changed from master to main January 21, 2021 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants